feat(rds): Add InstanceParameterGroup #2075
Conversation
| /** | ||
| * A instance parameter group | ||
| */ | ||
| export abstract class InstanceParameterGroupRef extends Construct { |
There was a problem hiding this comment.
We stopped using the Ref suffix an instead use interfaces. Lambda is a good example:
There was a problem hiding this comment.
These are imported but not used yet, so just removed them. fixed db1c8f3
| */ | ||
| protected validate(): string[] { | ||
| if (Object.keys(this.parameters).length === 0) { | ||
| return ['At least one parameter required, call setParameter().']; |
There was a problem hiding this comment.
Add unit tests for this case.
| /** | ||
| * The parameters in this parameter group | ||
| */ | ||
| parameters?: Parameters; |
There was a problem hiding this comment.
Why optional?
Is there a @default?
There was a problem hiding this comment.
No reason, just followed ClusterParameterGroup. I have no own opinion of this and change this to be required.
| */ | ||
| export interface InstanceParameterGroupProps { | ||
| /** | ||
| * Database family of this parameter group |
There was a problem hiding this comment.
Is there more detail? Perhaps a link to some documentation describing what a family is?
| this.setParameter(key, value); | ||
| } | ||
|
|
||
| this.parameterGroupName = resource.ref; |
There was a problem hiding this comment.
There should be a safe ref on the Resource, e.g. resource.parameterGroupName.
we already introduced `ImportedInstanceParameterGroup`
|
Thank you for review. I pushed some changes, so could you review them? |
| }); | ||
|
|
||
| for (const [key, value] of Object.entries(parameters || {})) { | ||
| this.setParameter(key, value); |
There was a problem hiding this comment.
Are you worried about using the passed in parameters object? Because you could do the following instead:
this.parameters = props.parameters || {};| */ | ||
| public setParameter(key: string, value: string | undefined) { | ||
| if (value === undefined && key in this.parameters) { | ||
| delete this.parameters[key]; |
There was a problem hiding this comment.
Don't overload the setParameter function with delete behavior. We don't support union types in public APIs because they don't translate well to other languages like Java, and it is also a confusing API to call set to delete something.
setParameter is for setting parameters and removeParameter is for removing them, keep it simple.
There was a problem hiding this comment.
@sam-goodwin I think this PR is based on the existing implementation of the cluster parameter group (https://github.com/awslabs/aws-cdk/blob/master/packages/%40aws-cdk/aws-rds/lib/cluster-parameter-group.ts implemented here #729) which proposes the same approach.
Does you comment mean that the cluster parameter group should also be refactored?
I'm currently working on a PR to support db instances and would like to have your opinion on this.
There was a problem hiding this comment.
On a second pass, I realize my comment about this union type was incorrect - string | undefined will work fine because it means optional. My mistake.
That said, it does seem confusing to me that we use setParameter to delete a parameter. I wouldn't expect to (for example) use map.put(key, null) to delete a key from a Map in Java. I was unaware of the precedent set in cluster parameter groups. What is the use case for this overloaded behavior?
There was a problem hiding this comment.
I don't know, it seems to come from this discussion #729 (comment).
For me it doesn't make sense to have setParameter and removeParameter methods, parameters should be defined directly in the construction props.
There was a problem hiding this comment.
I agree. @rix0rrr can you chime in?
There was a problem hiding this comment.
I think it might make sense to build the RDS parameters object incrementally, since it can be frking huge (there are many parameters to set!), so I think I might want some function to do:
lessDurabilityMorePerformance(paramGroup);
Or something. But I don't feel super strongly about this, I'm okay with removing the mutators as well.
| * Remove a previously-set parameter from this parameter group | ||
| */ | ||
| public removeParameter(key: string) { | ||
| this.setParameter(key, undefined); |
There was a problem hiding this comment.
... do the delete work here.
| }); | ||
|
|
||
| // THEN | ||
| expect(stack).to(haveResource('AWS::RDS::DBInstance', { |
There was a problem hiding this comment.
Should you also verify the value of the parameter instance group?
|
|
||
| // THEN | ||
| test.deepEqual(stack.node.resolve(exported), { parameterGroupName: { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' } }); | ||
| test.deepEqual(stack.node.resolve(imported.parameterGroupName), { 'Fn::ImportValue': 'Stack:ParamsParameterGroupNameA6B808D7' }); |
There was a problem hiding this comment.
Validate the stacks. expect(stack).to(haveResource(..)).
| new InstanceParameterGroup(stack, 'Params', { | ||
| family: 'hello', | ||
| description: 'desc', | ||
| parameters: {}, |
There was a problem hiding this comment.
I don't think we should allow you to create an invalid instance in the first place.
|
See #2187 where parameter groups are refactored and the changes from this PR could be integrated? |
Specific classes are exposed for a source database instance (`DatabaseInstance`), an instance created from a snapshot (`DatabaseInstanceFromSnapshot`) and a read replica instance (`DatabaseInstanceReadReplica`). Add construct for option groups and refactor parameter groups. Add basic support for instance event rules. Integration with Secrets Manager and secret rotation. Closes #2075 Closes #1693
refs. #1693
Pull Request Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.